Skip to content

fix: redirect postinstall script output to runtime output #2090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Mar 10, 2023
Merged

fix: redirect postinstall script output to runtime output #2090

merged 14 commits into from
Mar 10, 2023

Conversation

Bikappa
Copy link
Contributor

@Bikappa Bikappa commented Feb 27, 2023 •
edited
Loading

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Pipe the forked post install script logs into the parent process (the cli) logs

What is the current behavior?

Post install script logs are not visible when installing a core

What is the new behavior?

stdout and stderr streams of the post install script are shown in the cli stdout and stderr respectively

Does this PR introduce a breaking change, and is titled accordingly?

No, users should not expect to have structured logs from the cli

Other information

Fixes #1675

@Bikappa Bikappa force-pushed the fix/postinstall-output branch from b557895 to 66778a2 Compare February 27, 2023 21:37
@Bikappa Bikappa marked this pull request as ready for review February 27, 2023 21:44
@Bikappa Bikappa force-pushed the fix/postinstall-output branch from 6818d0c to 66c0fe0 Compare February 28, 2023 08:20
@Bikappa Bikappa force-pushed the fix/postinstall-output branch from 4acfd20 to 0929b19 Compare February 28, 2023 09:00
@Bikappa Bikappa self-assigned this Feb 28, 2023
@Bikappa Bikappa requested review from cmaglie and per1234 February 28, 2023 09:09
@codecov
Copy link

codecov bot commented Feb 28, 2023 •
edited
Loading

Codecov Report

Patch coverage: 26.08% and project coverage change: +0.08 🎉

Comparison is base (eece582) 36.46% compared to head (dbca859) 36.54%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2090      +/-   ##
==========================================
+ Coverage   36.46%   36.54%   +0.08%     
==========================================
  Files         229      229              
  Lines       19610    19624      +14     
==========================================
+ Hits         7151     7172      +21     
+ Misses      11618    11608      -10     
- Partials      841      844       +3
Flag Coverage Δ
unit 36.54% <26.08%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arduino/cores/packagemanager/install_uninstall.go 14.46% <26.08%> (+3.02%) ⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Bikappa Bikappa force-pushed the fix/postinstall-output branch from 986ee94 to abf2810 Compare March 2, 2023 12:30
@Bikappa Bikappa marked this pull request as draft March 2, 2023 12:36
@Bikappa Bikappa force-pushed the fix/postinstall-output branch from abf2810 to 4ae9826 Compare March 2, 2023 13:31
@Bikappa Bikappa force-pushed the fix/postinstall-output branch from 5d19485 to 1bc3537 Compare March 2, 2023 14:49
@Bikappa Bikappa force-pushed the fix/postinstall-output branch from c06fda3 to b7f2357 Compare March 2, 2023 15:19
@Bikappa Bikappa marked this pull request as ready for review March 2, 2023 16:11
@Bikappa Bikappa force-pushed the fix/postinstall-output branch from ad1fabf to 17ab168 Compare March 3, 2023 08:30
@umbynos umbynos self-requested a review March 6, 2023 15:26
cmaglie
cmaglie approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, let's wait for a test on the IDE by @ubidefeo :-)

ubidefeo
ubidefeo reviewed Mar 10, 2023
View reviewed changes
Copy link

@ubidefeo ubidefeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally sound.
I reported that while the Please run as root message is displayed to the user,
the installation is reported as finished, hence successful.

$> arduino-cli core install arduino:mbed_nano
Configuring platform....
Please run as root
...
Platform arduino:[email protected] installed
$>

Running

$> sudo arduino-cli core install arduino:mbed_nano
Platform arduino:[email protected] already installed

Will fail to reinstall the platform and run the post-install required installers.
This forces the user to uninstall and then sudo to install the platform again.

Platforms should receive an update to the script to trigger an exit code !=0 in case not invoked as root

cc @cmaglie @per1234 @facchinm

@cmaglie
Copy link
Member

cmaglie commented Mar 10, 2023

@ubidefeo I asked you to test on the IDE, we already tested the arduino-cli via command-line and it works fine AFAICT.

This forces the user to uninstall and then sudo to install the platform again.

This may be a practical procedure for the CLI, but I don't think the users from the IDE will be happy to run the whole IDE as root...
Also Not run as root is just one of the 1623 possible things that can go wrong.

Platforms should receive an update to the script to trigger an exit code !=0 in case not invoked as root

If your proposal is to un-do the installation (remove all the platforms/tools just installed) if the post-install script fails, then this is a much more involved change than the one proposed here.

@cmaglie
Copy link
Member

cmaglie commented Mar 10, 2023

Another idea could be to print something like:

WARNING: The configuration script returned an error (code 1):

  Not run as root

The platform is installed, but it may require the installation of drivers
or other manual intervention to work properly.

@Bikappa
Copy link
Contributor Author

Bikappa commented Mar 10, 2023

@cmaglie @ubidefeo I'd close this one if it's ok with you. Making the script fail and handling the failure is a different topic than showing its output.
For completeness, currently when the script fails (which is not the case for the example one in the issue), the following warning is shown:

Please run as root

WARNING cannot configure platform: exit status 1
Platform arduino:[email protected] installed

(I modified it here to make it exit with 1)

@Bikappa Bikappa merged commit fbeb271 into master Mar 10, 2023
@Bikappa Bikappa deleted the fix/postinstall-output branch March 10, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ubidefeo ubidefeo ubidefeo left review comments

@cmaglie cmaglie cmaglie approved these changes

@per1234 per1234 Awaiting requested review from per1234

@umbynos umbynos Awaiting requested review from umbynos

Assignees

@Bikappa Bikappa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Output from post install script is not printed
3 participants
@Bikappa @cmaglie @ubidefeo